Skip to content

Conversation

@cdesiniotis
Copy link
Contributor

@cdesiniotis cdesiniotis commented Oct 1, 2025

This change allows the use of drop-in config files for containerd and cri-o to be disabled when setting RUNTIME_DROP_IN_CONFIG to "". This allows better support for use cases such as microk8s where the top-level config may be a template that is rendered at runtime.

@coveralls
Copy link

coveralls commented Oct 1, 2025

Pull Request Test Coverage Report for Build 18199556959

Details

  • 13 of 27 (48.15%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.008%) to 37.17%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cmd/nvidia-ctk-installer/container/runtime/runtime.go 0 1 0.0%
pkg/config/engine/crio/crio.go 8 10 80.0%
pkg/config/engine/containerd/containerd.go 1 4 25.0%
pkg/config/engine/containerd/option.go 0 4 0.0%
pkg/config/engine/crio/option.go 0 4 0.0%
Totals Coverage Status
Change from base Build 18191171126: -0.008%
Covered Lines: 4964
Relevant Lines: 13355

💛 - Coveralls

@cdesiniotis cdesiniotis force-pushed the opt-out-of-drop-in branch 2 times, most recently from 8703143 to 51d85df Compare October 1, 2025 23:26
@cdesiniotis cdesiniotis marked this pull request as ready for review October 1, 2025 23:38
@elezar
Copy link
Member

elezar commented Oct 2, 2025

@cdesiniotis note that we explicitly distinguish between an empty value for RUNTIME_DROP_IN_CONFIG and the default value for a specific runtime. If a user sets RUNTIME_DROP_IN_CONFIG="" this should disable the use of drop-in configs meaning that an additional flag is not required.

Update: While adding a unit test to validate this behaviour, I noticed that this was not happening. I would still like to avoid adding an extra config option though. I have updated the PR. Let me know what you think.

Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment regarding RUNTIME_DROP_IN_CONFIG.

@elezar elezar force-pushed the opt-out-of-drop-in branch from 51d85df to 3449222 Compare October 2, 2025 11:32
@elezar elezar added this to the v1.18.0 milestone Oct 2, 2025
@elezar elezar force-pushed the opt-out-of-drop-in branch from 3449222 to 69d053c Compare October 2, 2025 12:31
Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to squash my commit on top and merge.

@elezar elezar changed the title Add ability to opt-out of containerd drop-in files Add ability to opt-out of drop-in files Oct 2, 2025
@cdesiniotis cdesiniotis force-pushed the opt-out-of-drop-in branch 2 times, most recently from ed4a363 to 2667197 Compare October 2, 2025 16:38
This change allows users to opt-out of using drop-in config files when configuring
containerd or crio. This is useful in cases where the top-level config is a
templated config file.

Signed-off-by: Christopher Desiniotis <[email protected]>
Co-authored-by: Evan Lezar <[email protected]>
containerd.WithRuntimeType(co.runtimeType),
containerd.WithUseLegacyConfig(co.useLegacyConfig),
containerd.WithContainerAnnotations(co.containerAnnotationsFromCDIPrefixes()...),
containerd.WithDisableDropInConfig(o.DropInConfig == ""),
Copy link
Contributor

@tariq1890 tariq1890 Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand the benefit of adding a new field here, if we can just directly evaluate this conditional internally when processing the runtime config

o.DropInConfig == ""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DropInConfig path is not passed to the configs themselves.

@ArangoGutierrez
Copy link
Collaborator

After comment #1251 (comment) , do we still want this patch?

@cdesiniotis
Copy link
Contributor Author

After comment #1251 (comment) , do we still want this patch?

No, I think we can hold off on this. Closing for now and we can revisit this if the need to disable drop-in files resurfaces.

@cdesiniotis cdesiniotis closed this Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants